-
Notifications
You must be signed in to change notification settings - Fork 94
implement isconvex for Pyramid #1266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1266 +/- ##
=======================================
Coverage 87.90% 87.91%
=======================================
Files 197 197
Lines 6240 6245 +5
=======================================
+ Hits 5485 5490 +5
Misses 755 755 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
juliohm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @cserteGT3! Should we export the new base function?
It's already exported. I chose the |
|
Could you please add some tests for the new |
JoshuaLampert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remarks/comments below.
|
Tests are failing due to some unrelated issue with DataStructures.jl 🍇 |
|
I've did similar updates, just added a comment on convexity and fixed a bug I introduced (using variable |
Co-authored-by: Joshua Lampert <[email protected]>
|
Added a new |
|
@cserteGT3 could you please add some tests for the |
Apart from running the formatter. |
|
Just added the test on GitHub directly 👍🏽 |
Good point! @cserteGT3 you can press Ctrl+Shift+I to get the formatting right. |
Thank you both for shaping this PR! Ran the tests locally, all passed. Regarding the formatting: I don't see that it would be different, than other files. Running "Format document" in VSCode doesn't change anything for me in |
test/predicates.jl is not formatted correctly, see https://github.com/JuliaGeometry/Meshes.jl/actions/runs/19064950241/job/54453184291?pr=1266#step:2:572. |
Thanks for the pointer! Figured out my VSCode setting and pushed the change. |
|
Opened an issue in StatsBase.jl, which is calling DataStructures.jl: JuliaStats/StatsBase.jl#974 I believe our only use of StatsBase.jl is for weighted sampling. We should seek alternatives to drop this dependency once and for all. |
juliohm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restarted the actions to see if they pass now. Other than that, the PR looks good to me.
|
Thank you @cserteGT3 ! Another one for the list 💯 🙂 |
|
Thank you for the quick merge and the collaborative efforts! |
This PR implements a specialized version for
isconvexforPyramid(as discussed in #1265). It assumes that the first four vertices construct the base.Fix #1265